This seems like an overkill. Serialization doesn't seem to require anything custom, so the attribute could instead be deserialize_with. But then the custom deserialization seems to only want to achieve having multiple names for the field, which can also be done with #[serde(alias = "currentAccumulator"].

Depending on which should be the serialization name for the field, you might want to use #[serde(rename = "currentAccumulator", alias = "accum"] instead.

The test json_rev_list_can_be_deserialized might also need to be expanded to make sure that the test vector, which includes currentAccumulator checks that des.accum.is_some(). as skip_serializing_if="Option::is_none" is set, it will silently fail if it can not interpret currentAccumulator correctly.

I wasn't trying to change the current deserialization, but it's pretty strange. It accepts values like:

{ ...
  "currentAccumulator": {
    "currentAccumulator": "1 000..."
  }
  ...
}

Probably because the visitor implements the visit_map method.

So do we think that's a mistake and it's just meant to support both currentAccumulator and accum as the field name? In that case it wouldn't need any custom deserialization.

I am fairly sure that it is a mistake. It was a custom implementation because we could not change it as it was a private field within Ursa (it has been a while so there might be more to it).

Well... The changes are introduced in this PR, so I was honestly expecting you to know the answer to that 😅 . To me it just seemed weird so I pointed it out.

I just moved the deserialization code around and changed it to an Accumulator value, the old implementation had the visit_map method.

Updated to just deserialize from a string, which is the format the tests use. I'm a little surprised the bitvec serialization is a list instead of hex bytes or something.